fixed - #64
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe token contract moves balances, allowances, and processed IDs to persistent storage with per-entry TTL refresh. Contract-wide instance state receives TTL updates on writes and through a new admin entrypoint. Tests and README documentation cover TTL behavior and operations. ChangesToken storage TTL
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Stellar-contracts-v1/README.md`:
- Around line 102-106: Update the README section explaining persistent() storage
to document that each balance and allowance entry has its own expiry, specify
the renewal period and that reads do not renew TTL, and describe the required
archival/restore and rent-funding procedure for expired entries.
In `@Stellar-contracts-v1/target/.rustc_info.json`:
- Line 1: Remove the generated target/.rustc_info.json artifact from version
control and update the repository ignore configuration to exclude the entire
target/ directory, preventing future Rust compiler metadata and local
environment paths from being committed.
In `@Stellar-contracts-v1/wpi-token/src/lib.rs`:
- Around line 156-158: Replace the expiring per-ID replay markers in the
redemption path around DataKey::ProcessedRedemption and the deposit path at
Stellar-contracts-v1/wpi-token/src/lib.rs:390-392 with durable replay
protection, such as an ordered source-chain checkpoint/nonce scheme that remains
enforceable for the full replay horizon. Update both sites consistently; do not
rely on periodic per-ID TTL renewal or expiring persistent storage.
- Around line 319-340: Update the balance and allowance migration flow around
read_balance, write_balance, and read_allowance_data to provide a one-time
snapshot-claim path that copies legacy instance entries into persistent storage
before instance expiry. Ensure inactive pre-upgrade accounts can claim their
existing state without requiring a future balance or allowance write, and make
the migration idempotent while preserving current persistent-state behavior.
In `@Stellar-contracts-v1/wpi-token/src/test.rs`:
- Around line 222-224: Update the test sequence around mint_from_deposit so
ledger time advances in intervals shorter than the instance TTL, calling
bump_instance_ttl before each instance-TTL deadline. Keep the instance alive
while aging the persistent entry, then assert that the older persistent balance
expires independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16fd58e1-b822-4f0f-8352-e9c0a4cc5b2d
📒 Files selected for processing (4)
Stellar-contracts-v1/README.mdStellar-contracts-v1/target/.rustc_info.jsonStellar-contracts-v1/wpi-token/src/lib.rsStellar-contracts-v1/wpi-token/src/test.rs
Comment on lines
+102
to
+106
| This matters because Soroban `instance()` storage has a single shared TTL for | ||
| the whole contract instance. If it is allowed to age out, every value stored in | ||
| that namespace becomes unavailable together. User-owned balances and allowances | ||
| therefore live in `persistent()` storage instead, so one stale account can no | ||
| longer drag every holder over the same expiry cliff. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document individual persistent-entry expiry and recovery.
Persistent storage avoids the shared instance cliff, but balances and allowances still expire after their own TTL if they are not written. State the renewal period, that reads do not renew it, and the required archival/restore and rent-funding procedure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Stellar-contracts-v1/README.md` around lines 102 - 106, Update the README
section explaining persistent() storage to document that each balance and
allowance entry has its own expiry, specify the renewal period and that reads do
not renew TTL, and describe the required archival/restore and rent-funding
procedure for expired entries.
| @@ -1 +1 @@ | |||
| {"rustc_fingerprint":6167044578903928928,"outputs":{"17747080675513052775":{"success":true,"status":"","code":0,"stdout":"rustc 1.88.0 (6b00bc388 2025-06-23)\nbinary: rustc\ncommit-hash: 6b00bc3880198600130e1cf62b8f8a93494488cc\ncommit-date: 2025-06-23\nhost: x86_64-pc-windows-msvc\nrelease: 1.88.0\nLLVM version: 20.1.5\n","stderr":""},"7971740275564407648":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\n___.lib\n___.dll\nC:\\Users\\pc\\.rustup\\toolchains\\1.88.0-x86_64-pc-windows-msvc\npacked\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"msvc\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""},"6027984484328994041":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\nlib___.a\n___.dll\nC:\\Users\\pc\\.rustup\\toolchains\\1.88.0-x86_64-pc-windows-msvc\noff\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"gnu\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""}},"successes":{}} No newline at end of file | |||
| {"rustc_fingerprint":9382291209947408725,"outputs":{"7971740275564407648":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\n___.lib\n___.dll\nC:\\Users\\Godsm\\.rustup\\toolchains\\1.88.0-x86_64-pc-windows-msvc\npacked\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"msvc\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""},"17747080675513052775":{"success":true,"status":"","code":0,"stdout":"rustc 1.88.0 (6b00bc388 2025-06-23)\nbinary: rustc\ncommit-hash: 6b00bc3880198600130e1cf62b8f8a93494488cc\ncommit-date: 2025-06-23\nhost: x86_64-pc-windows-msvc\nrelease: 1.88.0\nLLVM version: 20.1.5\n","stderr":""}},"successes":{}} No newline at end of file | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove generated compiler metadata from version control.
This target/.rustc_info.json file embeds the local Windows path C:\Users\Godsm\... and toolchain environment, making the repository environment-specific while exposing developer-identifying data. Remove it from the PR and ensure target/ is ignored.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Stellar-contracts-v1/target/.rustc_info.json` at line 1, Remove the generated
target/.rustc_info.json artifact from version control and update the repository
ignore configuration to exclude the entire target/ directory, preventing future
Rust compiler metadata and local environment paths from being committed.
Comment on lines
+156
to
+158
| let key = DataKey::ProcessedRedemption(redemption_id.clone()); | ||
| env.storage().persistent().set(&key, &true); | ||
| bump_persistent_ttl(env, &key); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not let replay-protection markers expire.
A processed ID is written once and then ages out after 180 days. Once absent, an old deposit can be minted again and an old redemption can be replayed. Use replay state that remains enforceable for the bridge’s full replay horizon, such as an ordered source-chain checkpoint/nonce design; periodic per-ID renewal is not sufficient at scale.
Stellar-contracts-v1/wpi-token/src/lib.rs#L156-L158: store redemption replay state with durable replay semantics.Stellar-contracts-v1/wpi-token/src/lib.rs#L390-L392: store deposit replay state with the same durable replay semantics.
📍 Affects 1 file
Stellar-contracts-v1/wpi-token/src/lib.rs#L156-L158(this comment)Stellar-contracts-v1/wpi-token/src/lib.rs#L390-L392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Stellar-contracts-v1/wpi-token/src/lib.rs` around lines 156 - 158, Replace
the expiring per-ID replay markers in the redemption path around
DataKey::ProcessedRedemption and the deposit path at
Stellar-contracts-v1/wpi-token/src/lib.rs:390-392 with durable replay
protection, such as an ordered source-chain checkpoint/nonce scheme that remains
enforceable for the full replay horizon. Update both sites consistently; do not
rely on periodic per-ID TTL renewal or expiring persistent storage.
Comment on lines
319
to
+340
| fn read_balance(env: &Env, address: &Address) -> i128 { | ||
| let key = DataKey::Balance(address.clone()); | ||
| env.storage() | ||
| .instance() | ||
| .get::<DataKey, i128>(&DataKey::Balance(address.clone())) | ||
| .persistent() | ||
| .get::<DataKey, i128>(&key) | ||
| .or_else(|| env.storage().instance().get::<DataKey, i128>(&key)) | ||
| .unwrap_or(0) | ||
| } | ||
|
|
||
| fn write_balance(env: &Env, address: &Address, amount: i128) { | ||
| let key = DataKey::Balance(address.clone()); | ||
| env.storage().persistent().set(&key, &amount); | ||
| bump_persistent_ttl(env, &key); | ||
| env.storage().instance().remove(&key); | ||
| } | ||
|
|
||
| fn read_allowance_data(env: &Env, owner: &Address, spender: &Address) -> Option<AllowanceData> { | ||
| let key = DataKey::Allowance(owner.clone(), spender.clone()); | ||
| env.storage() | ||
| .instance() | ||
| .set(&DataKey::Balance(address.clone()), &amount); | ||
| .persistent() | ||
| .get::<DataKey, AllowanceData>(&key) | ||
| .or_else(|| env.storage().instance().get::<DataKey, AllowanceData>(&key)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Migrate existing account state before instance expiry.
The fallback only preserves legacy balances/allowances until the shared instance namespace expires; inactive pre-upgrade accounts are never copied to persistent storage. Provide a one-time migration/snapshot-claim path before rollout rather than relying on each holder to perform a future write.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Stellar-contracts-v1/wpi-token/src/lib.rs` around lines 319 - 340, Update the
balance and allowance migration flow around read_balance, write_balance, and
read_allowance_data to provide a one-time snapshot-claim path that copies legacy
instance entries into persistent storage before instance expiry. Ensure inactive
pre-upgrade accounts can claim their existing state without requiring a future
balance or allowance write, and make the migration idempotent while preserving
current persistent-state behavior.
Comment on lines
+222
to
+224
| env.ledger() | ||
| .set_sequence_number(env.ledger().sequence() + PERSISTENT_ENTRY_TTL_EXTEND_TO - 10); | ||
| client.mint_from_deposit(&user_b, &11, &deposit_id(&env, 2)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep instance storage alive while aging the persistent entry.
This advances almost 180 days, but instance TTL is only 30 days; the contract instance expires well before mint_from_deposit on Line 224. Advance in intervals and call bump_instance_ttl before each instance-TTL deadline, then assert the older persistent balance expires independently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Stellar-contracts-v1/wpi-token/src/test.rs` around lines 222 - 224, Update
the test sequence around mint_from_deposit so ledger time advances in intervals
shorter than the instance TTL, calling bump_instance_ttl before each
instance-TTL deadline. Keep the instance alive while aging the persistent entry,
then assert that the older persistent balance expires independently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #4
Summary by CodeRabbit
New Features
Documentation
Tests